feat: add DPoP sender-constrained token support (RFC 9449)#109
feat: add DPoP sender-constrained token support (RFC 9449)#109davidbemer wants to merge 3 commits into
Conversation
Implements DPoP proof JWT validation so that DPoP-bound Descope session tokens (those with a cnf.jkt claim) are enforced on every request. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
Pull request overview
Adds server-side validation for DPoP sender-constrained session tokens (RFC 9449) to the PHP SDK, plus docs and unit tests, so applications can enforce cnf.jkt-bound tokens after verify().
Changes:
- Introduces
Descope\SDK\Token\DPoPto validate DPoP proof JWT structure, signature, and core claims (htm/htu/iat/ath) andcnf.jktthumbprint binding. - Exposes
DescopeSDK::validateDPoP()as a public wrapper for application use. - Documents the new flow in the README and adds PHPUnit tests for validation behavior and URL normalization.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/SDK/Token/DPoP.php | Implements DPoP proof validation, JWK import, signature verification, and claim checks. |
| src/SDK/DescopeSDK.php | Adds public validateDPoP() wrapper calling the DPoP validator. |
| src/tests/DPoPTest.php | Adds unit tests for DPoP validation and URL normalization cases. |
| README.md | Documents how to use validateDPoP() after verifying the session token. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $result = json_decode($decoded, true); | ||
| if (json_last_error() !== JSON_ERROR_NONE) { | ||
| throw new \Exception('Invalid JSON in JWT part: ' . json_last_error_msg()); | ||
| } |
|
|
||
| // Encode as ASN.1 INTEGER (prepend 0x00 if high bit set) | ||
| $r = ltrim($r, "\x00"); | ||
| if (ord($r[0]) > 0x7f) { |
| // Step 14-15: Import JWK and verify signature | ||
| $publicKey = self::importJwkAsPublicKey($jwk, $alg); | ||
| $signingInput = $parts[0] . '.' . $parts[1]; | ||
| $signature = self::base64urlDecode($parts[2]); | ||
| self::verifyDpopSignature($signingInput, $signature, $publicKey, $jwk, $alg); |
| // Steps 17-19: Validate required claims | ||
| if (empty($payload['jti'])) { | ||
| throw new \Exception('missing jti'); | ||
| } |
| * Uses openssl_public_decrypt with PKCS1_PSS padding via raw OpenSSL. | ||
| * PHP 7.x doesn't expose RSA-PSS directly in openssl_verify, so we | ||
| * use openssl_public_decrypt with OPENSSL_PKCS1_OAEP_PADDING as a | ||
| * workaround is NOT correct. Instead, PHP 8.x added support via EVP. | ||
| * For broad compatibility we attempt openssl_verify with the PSS digest; | ||
| * if not available, throw an informative error. | ||
| * | ||
| * @throws \Exception on verification failure or unsupported environment. | ||
| */ | ||
| private static function verifyPss(string $signingInput, string $signature, $publicKey, string $alg): void | ||
| { | ||
| // Map alg to hash algorithm name | ||
| $hashMap = ['PS256' => 'sha256', 'PS384' => 'sha384', 'PS512' => 'sha512']; | ||
| $hashAlg = $hashMap[$alg]; | ||
|
|
||
| // PHP 8.x: openssl_verify supports RSA-PSS via algorithm string | ||
| // We try using the OpenSSL algorithm identifier directly | ||
| $opensslAlgMap = [ | ||
| 'PS256' => defined('OPENSSL_ALGO_SHA256') ? 'SHA256' : null, | ||
| 'PS384' => defined('OPENSSL_ALGO_SHA384') ? 'SHA384' : null, | ||
| 'PS512' => defined('OPENSSL_ALGO_SHA512') ? 'SHA512' : null, | ||
| ]; | ||
|
|
||
| // Use openssl_public_decrypt with manual PSS verification | ||
| // Compute the digest of the signing input | ||
| $digest = hash($hashAlg, $signingInput, true); | ||
| $digestLen = strlen($digest); | ||
|
|
||
| // Decrypt the signature using the RSA public key (raw RSA operation) | ||
| $decrypted = ''; | ||
| $decryptResult = openssl_public_decrypt($signature, $decrypted, $publicKey, OPENSSL_NO_PADDING); | ||
| if (!$decryptResult) { | ||
| throw new \Exception('RSA-PSS signature verification failed (decrypt step)'); | ||
| } | ||
|
|
||
| // Verify PSS encoding manually |
| // Step 1-2: Check proof length | ||
| $dpopProof = trim($dpopProof); | ||
| if (strlen($dpopProof) > self::MAX_PROOF_LEN) { | ||
| throw new \Exception('DPoP proof exceeds maximum length'); | ||
| } | ||
|
|
||
| // Step 3: Require proof when token is DPoP-bound | ||
| if ($dpopProof === '') { | ||
| throw new \Exception('DPoP proof required: access token is DPoP-bound (cnf.jkt present)'); | ||
| } |
| /** | ||
| * Validate a DPoP proof for a DPoP-bound session token (RFC 9449). | ||
| * | ||
| * If the session token does not contain a cnf.jkt claim, this method | ||
| * returns immediately (the token is not DPoP-bound). | ||
| * | ||
| * @param string $sessionToken The session JWT (already verified). | ||
| * @param string $dpopProof The value of the DPoP HTTP header sent by the client. | ||
| * @param string $method The HTTP method of the incoming request (e.g. "GET", "POST"). | ||
| * @param string $requestUrl The full URL of the incoming request. | ||
| * @throws \Exception If the DPoP proof is missing or invalid. | ||
| */ |
| /** | ||
| * Unit tests for DPoP (RFC 9449) proof validation. | ||
| * | ||
| * These tests exercise the static helper methods directly without | ||
| * requiring a network connection or live Descope credentials. | ||
| */ | ||
| final class DPoPTest extends TestCase |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds RFC 9449 DPoP proof validation with a new DPoP class and a validateDPoP() wrapper on DescopeSDK. Cryptographic primitives (RSA/EC import, EMSA-PSS, thumbprint, htu normalization) look correct.
Sniffed out 4 issues:
- 2 🟡 MEDIUM: generic
\Exceptioninstead of typed SDK exceptions, no alg/kty consistency check - 2 🟢 LOW: asymmetric handling of zero-R in EC signature decode, README example doesn't guard
$_SERVER['HTTP_DPOP']
See inline comments for details. Good bones! Woof!
| string $method, | ||
| string $requestUrl | ||
| ): void { | ||
| DPoP::validateProof($dpopProof, $method, $requestUrl, $sessionToken); |
There was a problem hiding this comment.
🟡 MEDIUM: validateDPoP (and the underlying DPoP::validateProof) throws raw \Exception on every failure path.
The rest of the SDK uses typed exceptions — verify() throws AuthException / ValidationException. Callers can't catch (AuthException $e) to handle DPoP failures alongside other auth errors; they must catch the entire \Exception hierarchy, which swallows unrelated bugs.
Fix: wrap the throws in DPoP::validateProof with AuthException (signature/claim/format failures) and ValidationException (missing proof / arguments), matching the convention in Verifier.
| $result = openssl_verify($signingInput, $signature, $publicKey, $opensslAlg); | ||
| if ($result !== 1) { | ||
| throw new \Exception('DPoP proof signature verification failed'); | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM: No cross-check that alg matches jwk.kty.
Key import dispatches on kty (line 226), signature verification dispatches on alg (line 383). Nothing prevents e.g. alg: RS256 paired with jwk.kty: EC — the EC key gets imported, the kty === 'EC' branch on line 414 converts the signature, and openssl_verify happily verifies it as ECDSA-SHA256 because OpenSSL uses the key type, not the OPENSSL_ALGO_* constant, to pick the algorithm.
The thumbprint check at the end (line 154) binds the JWK to the session's cnf.jkt, so this isn't directly exploitable as an impersonation vector. But it violates RFC 9449 §4.3 ("the value of alg [...] indicates the algorithm of the asymmetric signature" — i.e. consistent with the key type) and mirrors the classic JWT alg-confusion vulnerability class. Add an explicit check, e.g.:
$expectedKty = match (true) {
str_starts_with($alg, 'RS'), str_starts_with($alg, 'PS') => 'RSA',
str_starts_with($alg, 'ES') => 'EC',
$alg === 'EdDSA' => 'OKP',
default => null,
};
if ($expectedKty !== null && ($jwk['kty'] ?? '') !== $expectedKty) {
throw new \Exception('alg ' . $alg . ' inconsistent with jwk.kty ' . ($jwk['kty'] ?? ''));
}| $r = ltrim($r, "\x00"); | ||
| if (ord($r[0]) > 0x7f) { | ||
| $r = "\x00" . $r; | ||
| } |
There was a problem hiding this comment.
🟢 LOW: Asymmetric handling of zero-value coordinates between R and S.
Lines 445-448 guard strlen($s) === 0 before ord($s[0]), but lines 441-443 don't do the same for $r. If $r is all-null after ltrim (degenerate/malformed signature), ord($r[0]) triggers a PHP 8 warning on uninitialized offset and the code emits 02 00 — an invalid ASN.1 INTEGER with zero-length content. OpenSSL rejects this with a less-than-helpful error.
| } | |
| $r = ltrim($r, "\x00"); | |
| if (strlen($r) === 0 || ord($r[0]) > 0x7f) { | |
| $r = "\x00" . $r; | |
| } | |
| $s = ltrim($s, "\x00"); | |
| if (strlen($s) === 0 || ord($s[0]) > 0x7f) { | |
| $s = "\x00" . $s; | |
| } |
| $descopeSDK->validateDPoP( | ||
| $sessionToken, // the verified session JWT | ||
| $_SERVER['HTTP_DPOP'], // DPoP header sent by the client | ||
| $_SERVER['REQUEST_METHOD'], // e.g. "GET" or "POST" |
There was a problem hiding this comment.
🟢 LOW: $_SERVER['HTTP_DPOP'] is unset when the client doesn't send a DPoP header. Passing null into validateDPoP(string $dpopProof, ...) raises TypeError rather than the intended \Exception('DPoP proof required...').
Suggest showing the safe form so users don't copy a snippet that crashes on the very case the function is designed to detect:
| $_SERVER['REQUEST_METHOD'], // e.g. "GET" or "POST" | |
| $_SERVER['HTTP_DPOP'] ?? '', // DPoP header sent by the client (empty string triggers a clear error) |
- Use TokenException (not \Exception) throughout DPoP.php for SDK consistency - Add is_array guard in base64urlDecodeJson to satisfy : array return type - Fix rawEcSigToDer to guard empty \$r after ltrim (same guard as \$s) - Add alg/kty cross-check: RS*/PS* require RSA, ES* require EC, EdDSA requires OKP - Remove dead \$opensslAlgMap array and stale comment in verifyPss - Update @throws docblock in DescopeSDK::validateDPoP to TokenException - Add DPoPTest.php to phpunit.xml test suite - Fix README example to use \$_SERVER['HTTP_DPOP'] ?? '' (null-safe) - Add class docblock note that jti replay protection is out of scope for stateless SDK Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
All review comments addressed in commit a8e70c2. Summary of changes: Fixed:
Deferred:
|
davidbemer
left a comment
There was a problem hiding this comment.
Inline replies to all review comments (details in the PR-level comment above).
| } | ||
| if (!is_array($result)) { | ||
| throw new TokenException('JWT part JSON did not decode to an array'); | ||
| } |
There was a problem hiding this comment.
Fixed. Added if (!is_array($result)) { throw new TokenException('JWT part JSON did not decode to an array'); } after the json_last_error() check. The function now guarantees it only returns a true PHP array.
| $publicKey = self::importJwkAsPublicKey($jwk, $alg); | ||
| $signingInput = $parts[0] . '.' . $parts[1]; | ||
| $signature = self::base64urlDecode($parts[2]); | ||
| self::verifyDpopSignature($signingInput, $signature, $publicKey, $jwk, $alg); |
There was a problem hiding this comment.
Fixed. Added an explicit alg/kty cross-check block before importJwkAsPublicKey: RS*/PS* algs require kty=RSA, ES* require kty=EC, and EdDSA requires kty=OKP. A TokenException is thrown on mismatch.
| // Steps 17-19: Validate required claims | ||
| if (empty($payload['jti'])) { | ||
| throw new TokenException('missing jti'); | ||
| } |
There was a problem hiding this comment.
Deferred — jti replay protection (RFC 9449 §11.1) is intentionally out of scope for a stateless SDK. This matches the go-sdk reference implementation. Added a note in the class docblock explaining the omission and directing callers to maintain their own jti store (e.g., a short-lived cache keyed on jti with TTL matching the iat window) if replay protection is required.
| // Step 3: Require proof when token is DPoP-bound | ||
| if ($dpopProof === '') { | ||
| throw new TokenException('DPoP proof required: access token is DPoP-bound (cnf.jkt present)'); | ||
| } |
There was a problem hiding this comment.
Fixed. All throw new \Exception( calls in DPoP.php have been replaced with throw new TokenException(, consistent with Extractor.php and Verifier.php. Added use Descope\SDK\Exception\TokenException; at the top of the file.
| * @param string $method The HTTP method of the incoming request (e.g. "GET", "POST"). | ||
| * @param string $requestUrl The full URL of the incoming request. | ||
| * @throws \Descope\SDK\Exception\TokenException If the DPoP proof is missing or invalid. | ||
| */ |
There was a problem hiding this comment.
Fixed. Updated @throws docblock to \Descope\SDK\Exception\TokenException to match what DPoP::validateProof() now throws.
| * These tests exercise the static helper methods directly without | ||
| * requiring a network connection or live Descope credentials. | ||
| */ | ||
| final class DPoPTest extends TestCase |
There was a problem hiding this comment.
Fixed. Added <file>src/tests/DPoPTest.php</file> to the test suite in phpunit.xml.
| $descopeSDK->validateDPoP( | ||
| $sessionToken, // the verified session JWT | ||
| $_SERVER['HTTP_DPOP'] ?? '', // DPoP header sent by the client (empty string if absent) | ||
| $_SERVER['REQUEST_METHOD'], // e.g. "GET" or "POST" |
There was a problem hiding this comment.
Fixed. Changed $_SERVER['HTTP_DPOP'] to $_SERVER['HTTP_DPOP'] ?? '' so the example handles the case where the DPoP header is absent without a PHP notice.
davidbemer
left a comment
There was a problem hiding this comment.
Follow-up inline replies for rawEcSigToDer and verifyPss.
| throw new TokenException('RSA-PSS signature verification failed (decrypt step)'); | ||
| } | ||
|
|
||
| // Verify PSS encoding manually |
There was a problem hiding this comment.
Fixed. Removed the unused $opensslAlgMap array and the stale comment describing an openssl_verify fallback. The function now only contains the actual working implementation: raw RSA public decrypt + manual EMSA-PSS verification.
| $result = openssl_verify($signingInput, $signature, $publicKey, $opensslAlg); | ||
| if ($result !== 1) { | ||
| throw new TokenException('DPoP proof signature verification failed'); | ||
| } |
There was a problem hiding this comment.
Fixed. Changed the $r guard from if (ord($r[0]) > 0x7f) to if (strlen($r) === 0 || ord($r[0]) > 0x7f), matching the identical pattern already applied to $s. All-zero input for $r will now safely prepend \x00 rather than triggering a warning on $r[0].
| string $method, | ||
| string $requestUrl | ||
| ): void { | ||
| DPoP::validateProof($dpopProof, $method, $requestUrl, $sessionToken); |
There was a problem hiding this comment.
Fixed. DPoP::validateProof() and all other throw sites in DPoP.php now throw TokenException (imported via use Descope\SDK\Exception\TokenException). The @throws docblock on validateDPoP() has been updated accordingly.
| $descopeSDK->validateDPoP( | ||
| $sessionToken, // the verified session JWT | ||
| $_SERVER['HTTP_DPOP'] ?? '', // DPoP header sent by the client (empty string if absent) | ||
| $_SERVER['REQUEST_METHOD'], // e.g. "GET" or "POST" |
There was a problem hiding this comment.
Fixed. Changed to $_SERVER['HTTP_DPOP'] ?? '' so the example handles the absent-header case without a PHP notice. Also updated the validateDPoP throws description to \Descope\SDK\Exception\TokenException.
- Updated composer.lock with phpunit/phpunit 12.5.25 (was 9.6.33) - Updated config.platform.php from 7.3.0 to 8.3.0 to match phpunit 12 requirements - Updated require.php from ^7.3 || ^8.0 to ^8.3 to align with phpunit 12 - Updated CI workflows to use PHP 8.3+ since phpunit 12 requires PHP >=8.3 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Fixed: updated Changes made:
|
Summary
src/SDK/Token/DPoP.phpimplementing RFC 9449 DPoP proof validation (signature verification for RSA, EC P-256/384/521, RSA-PSS, and EdDSA; JWK thumbprint check; htm/htu/iat/ath claim validation)DescopeSDK::validateDPoP()public method — a thin wrapper for server-side use afterverify(); no-op when the session token has nocnf.jktclaimsrc/tests/DPoPTest.phpwith PHPUnit tests covering valid RSA and EC proofs, all structural error paths, and URL normalisation edge casesMirrors the Go SDK implementation: descope/go-sdk#737
🤖 Generated with Claude Code